-
Notifications
You must be signed in to change notification settings - Fork 83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: standardize translation directory for i18n_tool #341
Conversation
Thanks for the pull request, @shadinaif! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
Makefile
Outdated
EXTRACTED_TEXT := $(EXTRACT_DIR)/text.po | ||
|
||
extract_translations: ## extract strings to be translated, outputting .po files | ||
cd $(WORKING_DIR) && i18n_tool extract | ||
mv $(EXTRACTED_DJANGO) $(EXTRACTED_TEXT) | ||
tail -n +20 $(EXTRACTED_DJANGOJS) >> $(EXTRACTED_TEXT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The plugin has no text in JS files. so collecting is failing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you referring to the plugin
directory, or to this repository as a whole?
We do have text in the xblock_lti_consumer.js Javascript file. Although we've recently observed that those strings have not been translated in two years; we haven't investigated yet.
Does removing this line mean that those Javascript strings will not be translated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a fix but then someone fixed it in a another PR https://github.com/openedx/xblock-lti-consumer/pull/353/files#diff-de32ac480ffc073d0eeb9f360281bd32ad2ad166d93821341685c945b414eb3c . So yes, we have JS translations
This PR needs openedx/xblock-lti-consumer#341 before it's merged. Refs: FC-0012 OEP-58
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the pull request. I left a few questions.
I also want to confirm that this won't impact existing translated strings. Those will remain in use until the time comes that openedx-atlas
is integrated into this repository. Is that correct?
As I mentioned in the comments, we noticed that our Javascript strings are not being translated properly. It may just be that Transifex hasn't committed (or isn't set up to commit) translations to this repository. But I want to be sure that we can still translate the strings in those files.
I also want to make you aware of the instructions in our README around pulling translations. When will this need to change? I tried to run it and got the following prompt, so I assume it's out of date in favor of the legacy bot. Let me know if I'm mistaken.
(xblock-lti-consumer) mroytman@C02FW0EAML85 xblock-lti-consumer % tx pull -f --mode=reviewed -l en,ar,es_419,fr,he,hi,ko_KR,pt_BR,ru,zh_CN
API token not found. Please provide it and it will be saved in '~/.transifexrc'.
If you don't have an API token, you can generate one in https://www.transifex.com/user/settings/api/
>
.tx/config
Outdated
@@ -2,8 +2,7 @@ | |||
host = https://www.transifex.com | |||
|
|||
[o:open-edx:p:xblocks:r:lti-consumer] | |||
file_filter = lti_consumer/translations/<lang>/LC_MESSAGES/text.po | |||
source_file = lti_consumer/translations/en/LC_MESSAGES/text.po | |||
file_filter = lti_consumer/conf/locale/<lang>/LC_MESSAGES/text.po |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will still extract translations to this repository. In the grand scheme of the project, is the idea that eventually the source files in this directory will be removed from this repository and will instead be committed to openedx-translations
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it will extract it to this repo but the destination directory is ignored by .gitignore
. daily Action is triggered in openedx-translations
to collect translations from all supported repos, so the generated text.po
will be copied to openedx-translations
during the Action execution
Resolved the conversation with an errant click, unresolved and ran that tests. |
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## master #341 +/- ##
=======================================
Coverage 97.68% 97.68%
=======================================
Files 77 77
Lines 6187 6187
=======================================
Hits 6044 6044
Misses 143 143
Flags with carried forward coverage won't be shown. Click here to find out more. Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
Hi @MichaelRoytman! Would you mind taking a look at this? Thanks! |
Hi, @mphilbrick211! I took a look a few weeks ago and left a few questions for Shadi. If they could address my remaining questions, I'd be happy to re-review. I want to confirm the impact on Javascript string translations and on existing translations in the repository. Thank you. |
3 similar comments
Hi, @mphilbrick211! I took a look a few weeks ago and left a few questions for Shadi. If they could address my remaining questions, I'd be happy to re-review. I want to confirm the impact on Javascript string translations and on existing translations in the repository. Thank you. |
Hi, @mphilbrick211! I took a look a few weeks ago and left a few questions for Shadi. If they could address my remaining questions, I'd be happy to re-review. I want to confirm the impact on Javascript string translations and on existing translations in the repository. Thank you. |
Hi, @mphilbrick211! I took a look a few weeks ago and left a few questions for Shadi. If they could address my remaining questions, I'd be happy to re-review. I want to confirm the impact on Javascript string translations and on existing translations in the repository. Thank you. |
Oof. Github is having issues and posted my comment four times. Sorry for tagging you four times! 😅 |
Hi @shadinaif! Friendly ping on this :) |
Hi @MichaelRoytman and @mphilbrick211 , thank you for your patience on this. I'll push an update and address the issues above very soon. We decided to finish the project without introducing any breaking changes until the last phase of it. I'll add a detailed description to the PR, we're working on a template for that Thank you! |
fcb6336
to
eebbd79
Compare
eebbd79
to
4a210a8
Compare
This PR needs openedx/xblock-lti-consumer#341 before it's merged. Refs: FC-0012 OEP-58
e667807
to
7d9fa7d
Compare
@MichaelRoytman please review now, tested on devstack olive, works fine. We're not changing any behavior in this phase, please see the details in the PR description. Thank you! |
This PR needs openedx/xblock-lti-consumer#341 before it's merged. Refs: FC-0012 OEP-58
@shadinaif Looks like there are some test failures. |
Thank you @e0d . The error is related to a missing token for codecov.io . It's either deleted by someone, or affected by latest Github incident and needs to be re-set https://github.blog/2023-03-23-we-updated-our-rsa-ssh-host-key/ |
Refs: FC-0012 OEP-58
7d9fa7d
to
e4bb599
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me!
I pinged @Zacharis278 to take a quick look, since he's done some translations works in this repository recently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All tests passed! Looks like this is good to go!
Merging PR on behalf of Shadi, as it has 3 approvals and looks good to me. |
@shadinaif 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
This PR needs openedx/xblock-lti-consumer#341 before it's merged. Refs: FC-0012 OEP-58
feat: standardize translation directory for i18n_tool
This PR prepares the repository to comply with openedx-translations by doing the following:
conf/locale
and converttranslations
into a symbolic link toconf/locale
. The reason behind that is to comply with openedx-translations mechanism that requires translation files to be inconf/locale
directory. This fix will avoid the need to modifyfind
commands herelocale
link is removed as it is not needed anymoremake extract_translations
build a file nameddjango.po
instead oftext.po
. And create a symbolic link fromtext.po
todjango.po
. This is because openedx-translations requiresdjango.po
as the name of the file. And it must be a valid file (not a link) because it usesgit add
hereReferences
This pull request is part of the FC-0012 project which is sparked by the Translation Infrastructure update OEP-58.
Check the links above for full information about the overall project.
Internalization is being rearchitected in Open edX Python, XBlock, Micro-frontend, and other projects. There are a number of immediately visible changes:
.json
,.po
or.mo
files will be committed into the repos.make extract_translations
in all repositoriesBreaking Changes
One of the primary goals of the project is to avoid breaking changes. If you noticed any suspicious code, please raise your concern. But before that, please know the strategy we're following to avoid breaking changes
For XBlocks:
conf/locale
. Therefore, we are creating a link fromconf/locale
totranslations
so Atlas can find the path without disrupting the current way of having translations locally within the XBlocks